Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V5 migration of get-balance (native) and get-all-events api #810

Closed
wants to merge 12 commits into from

Conversation

nischitpra
Copy link
Contributor

@nischitpra nischitpra commented Dec 11, 2024

Unit tests are created for compairing v4 and v5 outputs.
I'll need some basic help with Typescript types. I've only made jsdoc comments to explain the function parameters.

APIs migrated

  • get-balance
  • get-all-events

PR-Codex overview

This PR primarily focuses on code quality improvements, including type adjustments, error handling enhancements, and updates to logging messages. It also includes some updates to dependencies and modifications to the way certain functions are structured.

Detailed summary

  • Added text=auto to .gitattributes.
  • Updated error handling to use _e instead of e in multiple files.
  • Changed message formatting in several log statements.
  • Adjusted type imports to use type keyword where applicable.
  • Refactored function parameters to use unknown instead of any.
  • Updated package.json version for @thirdweb-dev/engine.
  • Improved consistency in using const instead of let where possible.
  • Modified handler signatures to use _req instead of req in various routes.
  • Added a new testWebhook method in WebhooksService.
  • Enhanced compatibility for chain ID handling in multiple services.

The following files were skipped due to too many changes: sdk/src/services/AccountFactoryService.ts, tests/unit/migrationV5.test.ts, sdk/src/services/ContractRolesService.ts, sdk/src/services/ContractRoyaltiesService.ts, sdk/src/services/MarketplaceOffersService.ts, sdk/src/services/AccountService.ts, sdk/src/services/MarketplaceEnglishAuctionsService.ts, sdk/src/services/BackendWalletService.ts, sdk/src/services/MarketplaceDirectListingsService.ts, sdk/src/services/DeployService.ts, sdk/src/services/Erc20Service.ts, sdk/src/services/Erc721Service.ts, sdk/src/services/Erc1155Service.ts, yarn.lock

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

const returnData = mapEventsV4ToV5(
await getContractEvents({
contract: contract,
fromBlock: BigInt(fromBlock),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript error here.
I think we should update the querystring schema here to be int | undefined only, and here use maybeBigInt(fromBlock) which maps a number | undefined type to bigint | undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fromBlock: maybeBigInt(fromBlock?.toString()) instead but keeping it unresolved to make sure its done correctly

src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
tests/unit/migrationV5.test.ts Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
src/server/routes/contract/events/get-all-events.ts Outdated Show resolved Hide resolved
@nischitpra nischitpra force-pushed the np/balanceOfV5 branch 3 times, most recently from 45b9d08 to 8dd2de0 Compare December 12, 2024 06:37
@nischitpra nischitpra enabled auto-merge (squash) December 12, 2024 08:51
@nischitpra nischitpra closed this Dec 12, 2024
auto-merge was automatically disabled December 12, 2024 09:24

Pull request was closed

@nischitpra nischitpra deleted the np/balanceOfV5 branch December 12, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants